Skip to content

fix(duckdb): fail fast on unresponsive remote Quack attach - #1164

Merged
wesm merged 1 commit into
kenn-io:mainfrom
mjacobs:fix/quack-attach-timeout
Jul 23, 2026
Merged

fix(duckdb): fail fast on unresponsive remote Quack attach#1164
wesm merged 1 commit into
kenn-io:mainfrom
mjacobs:fix/quack-attach-timeout

Conversation

@mjacobs

@mjacobs mjacobs commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

A native Quack attach (quack:HOST:PORT) against an unresponsive endpoint
hangs indefinitely — observed in practice when the target is reachable at
the TCP level but never completes the protocol handshake (for example a
plain-HTTP listener on a non-loopback address, which the extension contacts
over TLS). Because the hang is inside the DuckDB extension, every command
that attaches a remote mirror (duckdb push, duckdb status,
duckdb serve with a remote URL) blocks forever with no error.

This makes the remote attach path fail fast instead:

  • A TCP preflight (net.DialTimeout) against the host:port extracted from
    the quack URL catches unreachable or blackholed endpoints before the
    extension is even loaded. The address extraction handles the native
    quack:host:port form, IPv6 brackets, userinfo, and URL-scheme forms
    with default ports.
  • A watchdog around the ATTACH itself runs the statement in a goroutine and
    selects on completion versus a timer, returning an error that names the
    endpoint and the timeout knob. This covers the observed repro shape:
    TCP connects, then the handshake stalls.

The timeout is configurable as [duckdb].attach_timeout /
AGENTSVIEW_DUCKDB_ATTACH_TIMEOUT, defaulting to 20s. A negative value
disables the guard; zero means "use the default" because a TOML duration
zero-value is indistinguishable from unset.

Tradeoffs: a timed-out ATTACH may leak its goroutine and connection, since
the extension call cannot be interrupted from Go — accepted and documented
in a code comment, as callers treat a failed attach as fatal. Scope is the
initial attach only; the existing stale-connection reattach retry path
during a live session is unchanged. Local file opens are untouched.

Where to look: internal/duckdb/connect.go (preflightQuackDial,
runWithAttachTimeout, openQuackClient), the knob registration in
internal/config/config.go, and docs/duckdb.md for the documentation.

@roborev-ci

roborev-ci Bot commented Jul 17, 2026

Copy link
Copy Markdown

roborev: Combined Review (adb768b)

Medium-severity issues remain in DuckDB timeout handling, platform error ordering, and configuration precedence.

Medium

  • internal/duckdb/connect.go:385 — Timed-out CGO attach operations are abandoned under the assumption that the process exits immediately. Daemon-delegated duckdb push --watch retries can instead leak a goroutine, SQL connection, and native resources after every timeout. Ensure timed-out work can be terminated or isolated in daemon mode, bound retry/resource usage, and add a repeated-timeout daemon/watch regression test.

  • internal/duckdb/connect.go:325 — TCP preflight runs before openDuckDB, causing Windows/ARM64 to return a network error instead of errUnsupportedPlatform when the endpoint is unavailable, breaking TestNewQuackStoreReportsUnsupportedPlatform. Check platform/driver availability first, then close the initialized database if preflight fails.

  • internal/config/config.go:1120AGENTSVIEW_DUCKDB_ATTACH_TIMEOUT=0 cannot override a nonzero file setting because zero is treated as equivalent to an unset environment variable. Track explicit environment-variable presence separately from its parsed value, and add precedence tests for zero, positive, and negative values.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m43s

- feat(duckdb): fail fast on unresponsive remote Quack attach
- docs(duckdb): document attach_timeout knob
@wesm
wesm force-pushed the fix/quack-attach-timeout branch from adb768b to 509c052 Compare July 23, 2026 11:03
@roborev-ci

roborev-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (509c052)

No Medium, High, or Critical findings were identified.


Reviewers: 2 done | Synthesis: codex, 5s | Total: 4m20s

@wesm
wesm merged commit 537aeeb into kenn-io:main Jul 23, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants